refactor(runtime): remove obsolete permission mode compatibility path - #5300
chinawch007 wants to merge 3 commits into
Conversation
Remove SessionManager.setPermissionMode, its legacy-store fallback, and helpers used only by that path. Production Desktop and CLI permission changes continue through the versioned configuration authority. Migrate concurrency, Deep Research cleanup, and pending Interaction tests to transitionSessionConfiguration. Document the paired optional Store capabilities and verify missing capabilities reject without fallback writes. Fixes apache#4795 Generated-by: Codex
5d5e674 to
1d845e4
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Reviewed at head 1d845e4. I re-verified the premise on current main: SessionManager.setPermissionMode has no production caller (Desktop sessions:setPermissionMode and the CLI driver both go through session.configuration.update → transitionSessionConfiguration), and the removed legacy fallback was not a faithful mirror of the authority (no revision fence, no archived check, non-atomic header + boundary writes). So the removal is a real entropy reduction, and transitionSessionConfiguration covers every check the deleted helpers had. Both production paths are unaffected. Lint, format and the compiled session-manager suite (191/191) pass on this head; the migrated tests also pass on main, which matches what the description says.
Two things I'd like to see before this lands, both about the one authority acceptance criterion in #4795:
-
setExecutionBoundaryKindis the same shape as the path this PR deletes.packages/runtime/src/session-manager.ts:1653-1688derives apermissionModeitself and persists it throughstore.setExecutionBoundaryKind(...), which insqlite-session-metadata-store.ts:4889-4899patchespermissionModeandlabelsinto the header with no version fence, and without the deep-research cleanup, archived check or revision check thattransitionSessionConfigurationapplies. It also has zero production callers (the only non-test caller,run-command-core.ts:343, resolves toRuntimeHostRunRuntime, which forwards to the driver's configuration update). The test file now promotes it to a'direct'route, which gives it a longer life. By the issue's own standard I think it should go in the same change, or the PR should say why it can't (I couldn't find a blocker: same file, same policy, same caller count). -
The description says capabilities stay optional, but not why. The reason is checkable and worth writing down:
SessionAuthorityStorealready requiresreadHeaderRecordSnapshotandupdateSessionConfiguration, the only production store implements both, and optionality only serves a handful of test fixtures. Making them required would also touchrelocateSessionWorkspace, which sharesrequireSessionConfigurationStore, so keeping them optional here is the right scope. Saying that answers the review item in #4795 instead of stepping around it.
Line-level notes are inline. One small correction for the description: the migrated tests pass on main unchanged, so the "fail without it" box should stay unticked, which is fine for a pure removal.
AI assistance: I used Claude Code to survey callers and run the ablation; every finding above was checked by me against the code and the test run.
| // Either the configuration revision or the boundary revision can fence | ||
| // the stale request, depending on when its snapshot was observed. | ||
| if ( | ||
| route === 'configuration' && |
There was a problem hiding this comment.
P2. With each request reading its own snapshot inside update(), the two requests are no longer guaranteed to observe the same revision, so the assertion accepts either SessionConfigurationRevisionConflictError or operation_conflict. Those map to different client-visible outcomes at the Host (configurationSuccess(revisionConflict) vs configurationFailure), so the test no longer pins which one a stale request gets. The old 'legacy' route was deterministic. Suggest reading both snapshots before Promise.allSettled so both requests share one expectedRevision, then asserting the revision-conflict branch only. (The comment above about both requests observing Explore also no longer matches the code.)
| for (const capability of missing) { | ||
| Object.defineProperty(store, capability, { value: undefined }); | ||
| } | ||
| store.updateHeader = async () => assert.fail('Must not fall back to header writes'); |
There was a problem hiding this comment.
P3. requireSessionConfigurationStore is a single if (!a || !b), so the three missing-capability cases exercise the same throw; the third is the union of the first two. One case is enough. The two assert.fail probes are also unreachable now: transitionSessionConfiguration throws at its first line before any store write could happen. The valuable part of this block is that fallback writes are forbidden, and one case says that.
| readHeaderRecordSnapshot?(sessionId: string): Promise<VersionedSessionHeader>; | ||
| /** | ||
| * Atomically check the expected revision and commit configuration, execution | ||
| * boundary and the new revision. Requires readHeaderRecordSnapshot. |
There was a problem hiding this comment.
P3. This second paragraph restates the first (capabilities may be absent, no unversioned fallback). The first paragraph is the one that answers #4795; this one can go.
| return headerToSummary(next); | ||
| } | ||
|
|
||
| async setExecutionBoundaryKind( |
There was a problem hiding this comment.
See the top-level comment: this method persists permissionMode outside transitionSessionConfiguration with none of its checks and has no production caller. Same shape as the path this PR removes.
Remove the unused SessionManager boundary setter and its unversioned Store requirement. Migrate descendant revocation and admission checks to configuration authority and remove obsolete direct-route fixtures. Split stale configuration revisions from gated concurrent boundary conflicts, asserting each error precisely. Simplify capability tests and explain Runtime optionality alongside the atomic Store contract. Refs apache#4795 Generated-by: Codex
Overall reviewThanks for checking this. I agree that retaining I migrated the descendant shell-revocation and missing-admission-authority tests to The revised description now explains why the Store capabilities remain optional: production After rebuilding, all 559 affected tests passed. Full lint, format, build, typecheck, and Desktop/UI knip checks also passed. The full workspace test suite and manual Desktop interaction were not run. Codex assisted with the implementation, verification, and drafting these replies; the follow-up commit retains its Concurrent update errorsAgreed that accepting either error obscured the test contract. I split this into two deterministic cases:
One detail I found when implementing the suggestion: pre-reading the configuration snapshot alone does not guarantee a configuration-revision error. Inside the serialized commit, the boundary revision is checked before the second configuration-revision check. The explicit read gate fixes that interleaving, while the separate stale-snapshot case pins the revision-conflict result. No production error ordering or Host mapping changed. Missing Store capabilitiesRemoved the redundant “both missing” case and both Store contract commentsCombined the comments into one contract block. It explains the paired capabilities, why Runtime keeps them optional despite the production authority requiring them, and the absence of a fallback. I retained the atomic expected-revision/configuration/boundary requirement because that is a separate implementation obligation, rather than a restatement of optionality. Remaining boundary setterRemoved |
github-actions
Bot
added
effort/L
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at head b844456f against main (ea990ca). Comment only.
Premise and blast radius — verified
The removed path was unreachable in production.
SessionManager.setPermissionModehas no production caller. Desktop reaches permission changes throughsessions:setPermissionMode→updateConfiguration(apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts:167-169), and the CLI driver'ssetPermissionModeis a different method onSessionDriverthat already routes throughsession.configuration.update(packages/cli/src/runtime-host-session-driver.ts:683-698).- The only non-test
setExecutionBoundaryKindcall site,packages/cli/src/run-command-core.ts:343, is onMakaRunRuntime(run-command-core.ts:66), whichruntime-host-run-command.ts:371-373forwards to the driver's configuration update. Neither name survives anywhere inpackages/**/srcoutside tests. - The branch condition was keyed on store capabilities, not on persisted session state, and the only production store (
SessionAuthorityStore, wired atpackages/storage/src/execution-stores.ts) implements bothreadHeaderRecordSnapshotandupdateSessionConfiguration. No stored flag, column or legacy row could have selected the old branch.
No on-disk state depended on it. permissionMode and the boundary rows are read identically by the surviving authority, and nothing in the read/migration path changed. The legacy-state coverage that matters still exists and is untouched: the v1 WorkHub coordination repair persists a legacy-shaped header (toolProfile: workhub-coordination-v1 + explore) straight through Storage and repairs it via transitionConfiguration (packages/runtime-host/src/server/workhub-coordination-coordinator.ts:1050-1070), and the read model still projects legacy persisted mode_change system notes (session-manager.test.ts:9659-9672). packages/runtime is private: true, so there is no published-surface concern either.
Removal is complete within Runtime. No setPermissionMode, setPermissionModeWithLegacyStore, sessionConfigurationWithPermissionMode or executionBoundaryMatchesPermissionMode references remain in session-manager.ts; no route === 'direct' fixtures or "compatibility bridge" copy remain in the tests; and no new unused imports were introduced (the import-only symbols in the test file, e.g. createWorkspaceWritePermissionProfile, were already import-only on main).
Coverage migration is net-neutral to better. Name-diffing the test file base → head: the three deleted names (temporarily preserves setPermissionMode for legacy SessionStore implementations, the legacy concurrency route, the direct boundary route on Explore revocation) are replaced by configuration changes require Store capability: *, configuration authority rejects a stale revision…, serializes configuration commits that observed the same execution boundary and configuration narrowing revokes descendant background shell authority. Widening during an active Turn is still pinned (assert.strictEqual(backend?.stopCalls, 0) at session-manager.test.ts:4821), and pending-Interaction rejection, Deep Research label cleanup and descendant revocation all moved onto transitionSessionConfiguration. The missing-capability tests are now one case per capability and assert operation_unavailable plus an unchanged snapshot and boundary, which is the property that actually matters (no silent fallback write).
The doc comment at session-manager.ts:644-651 is accurate about why the pair stays optional, and splitting the stale-revision test from the boundary-fence test is the right shape.
Nits
-
configurationForHeaderdropsexecutorId(session-manager.test.ts:14901). The helper it replaces (sessionConfigurationWithPermissionMode) passedexecutorId: header.executorId, andsessionConfigurationMatchesExceptPermissionModecomparesexecutorId. For a plugin-executor-backed session the migrated tests would therefore silently computepermissionModeOnly === falseand exercisecommitExecutionResourceTransitionrather than thecommitExecutionBoundaryTransitionpath the test names describe. Every current fixture isai-sdk, so nothing is wrong today — but this migration is the cheap moment to keep the helper field-complete. -
The dropped
AtomicBoundaryMemorySessionStoreinvariant has no replacement. ItsupdateHeaderthrewpermissionMode must be projected by the boundary transitionwhenever anything outside a boundary commit patchedpermissionMode. Nothing in the head tests asserts that property any more, even thoughVersionedConfigurationMemorySessionStore.updateSessionConfiguration(session-manager.test.ts:14181) performs exactly the two-step write it was guarding against. A few lines of guard there would keep #4795's "one authority" property pinned at the store double. -
Storage still exposes the same shape this PR deletes — follow-up, not a blocker.
setExecutionBoundaryKindremains on the authority store handed to Runtime (packages/storage/src/execution-stores.ts:438), onSessionStore(packages/storage/src/session-store.ts:919), and assetExecutionBoundaryKindSync(packages/storage/src/sqlite-session-metadata-store.ts:4796), which still projectspermissionMode/labelsinto the header (sqlite-session-metadata-store.ts:4889-4893). After this PR nothing in production calls it. I think leaving it is the right scope here (separate contract, own tests), but a one-line note that Storage is deliberately out of scope would close out #4795's acceptance criterion rather than leaving it half-met.
AI use: generative tooling helped me survey callers and diff base/head test names; every claim above was checked against the code at b844456f.
Preserve executorId when migrating permission changes to configuration authority and cover widening with an active plugin-executor Turn. Reject permissionMode patches through ordinary and versioned header updates in the configuration Store double. Keep header and boundary projection inside configuration commits and remove the unused memory boundary setter and duplicate header write. Verify the executor regression fails without the fix and injected header permission writes are rejected. All 560 affected tests and the prescribed lint, format, build, typecheck, and knip checks pass. Refs apache#4795 Generated-by: Codex
|
Thanks for checking the production callers and the migrated coverage. I've addressed the two test issues in
Validation: all 560 tests across the same nine affected test files passed after rebuilding, along with lint, format, full build, typecheck, and both prescribed knip checks. The full workspace test suite and manual Desktop interaction were not run. AI use: Codex assisted with the implementation, verification, and this reply draft; the follow-up commit includes |
2 participants
Summary
Fixes #4795
Remove the unused
SessionManager.setPermissionModewrapper, its legacy-store fallback, and the unusedSessionManager.setExecutionBoundaryKindentry point. Desktop and CLI continue changing permissions throughsession.configuration.update→transitionSessionConfiguration, leaving one Runtime authority for persisted permission changes.executorIdin migrated test configurations and verify permission widening with an active plugin-executor Turn. Restore the test Store's guard against ordinary or versioned header writes that bypass configuration authority.SessionAuthorityStorerequires both. Document the atomic revision/configuration/boundary contract and test each missing capability.Storage's public boundary mutation API is deliberately outside this Runtime cleanup. Its private
setExecutionBoundaryKindSyncremains in use by the versioned configuration transaction.Production permission switching and same-configuration no-op behavior are unchanged. Historical
mode_changewrites were removed in #4879; current boundary logging remains unchanged.Verification
Passed locally at
239da0784after rebuilding:npm run lint,npm run format:check,npm run build, andnpm run typecheck.knip --workspace apps/desktopandknip --workspace packages/ui, using the installed repository binary.session-manager,runtime-kernel-interaction,session-manager-terminal-ledger, andruntime-event-read-model; Hostsession-catalog-coordinatorandexecution-model-composition; Storagesqlite-session-metadata-store; CLIruntime-host-session-driver; Desktopruntime-host-client-uds. These include Host permission regressions for ordinary Turns and active Goal continuations.executorIdcopy makes the new regression test fail withsession_busy. Injecting ordinary or versioned header permission writes fails at the restored Store guard; disabling that guard makes the ordinary bypass pass again. All temporary mutations were restored before the final regression run.Not run: the full workspace test suite or manual Desktop interaction. Existing behavioral tests preserve the API-removal semantics; the failing-before check specifically covers the executor-preservation correction.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the cleanup and review follow-ups, migrated and extended tests, ran verification, and drafted this description. Affected commits include
Generated-by: Codex; retain the trailer in the final squash commit.Checklist
Does this PR entail a change in behavior?